fix(prom/scrape): recreate internal scraper after reloaded #5541
Conversation
Signed-off-by: hainenber <[email protected]>
Signed-off-by: hainenber <[email protected]>
hainenber
force-pushed
the
allow-reload-internal-prom-scraper
branch
from
f2d22dc to
53e2238
Compare
| // Update created component with prometheus.Fanout and prometheus.ScrapeManager | ||
| data, err = o.GetServiceData(http.ServiceName) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get information about HTTP server: %w", err) | ||
| } | ||
| httpData := data.(http.Data) | ||
| flowAppendable, scraper := c.createPromScrapeResources(httpData, args) | ||
| c.appendable = flowAppendable | ||
| c.scraper = scraper |
There was a problem hiding this comment.
Is this necessary, if the same code will run in Update on line 175?
There was a problem hiding this comment.
I was trying to retain the previous setup. Now you're saying, it does look extra. Let see if the upcoming unit test can invalidate this step and I'll remove it
| } | ||
| newFlowAppendables, newScraper := c.createPromScrapeResources(data.(http.Data), newArgs) | ||
| c.appendable = newFlowAppendables | ||
| c.scraper = newScraper |
There was a problem hiding this comment.
There's an issue here: the Run method runs the original c.scraper, and will never terminate that scraper/run the new one that's created. This means the changes here on subsequent updates will never truly apply.
There was a problem hiding this comment.
I totally missed this one out 🫣
But I do think we can rewrite the scraper's Run goroutine into a for-select loop to handle scraper restart.
Let me try out my luck on this.
| return fmt.Errorf("failed to get information about HTTP server: %w", err) | ||
| } | ||
| newFlowAppendables, newScraper := c.createPromScrapeResources(data.(http.Data), newArgs) | ||
| c.appendable = newFlowAppendables |
There was a problem hiding this comment.
I don't think we have to recreate the appendables; the call to UpdateChildren on line 240 should be sufficient, and we can return to having only one instance of c.appendable for the lifetime of the component.
There was a problem hiding this comment.
Sounds great, -1 work for me then 😄
|
Alrighty, I'll try writing one 😄 |
|
@hainenber You might find the componenttest package useful here to test components (Look for other unit tests which use it to get examples). This will handle running/updating the component for you so you can focus your unit test on creating a fake scrape target and determining whether protobuf negotiation is actually enabled. You'll know protobuf negotation is being used by looking at the |
|
Hi there, I've found making the goroutine in Run() function reloadable pretty tough and think it's much better to fix this on upstream, namely I've made a PR to address it. Once the patch got merged, hopefully someone can cherry-pick it into a new For now, this PR will be closed in anticipation. |
|
Turns out the The new option, My PR on |
github-actions
bot
added
the
frozen-due-to-age
PR Description
Recreate internal scrapeManager of
prometheus.scrapeFlow component and hence multiple options will be applied once getting reload signalWhich issue(s) this PR fixes
Fixes grafana/alloy#329
Notes to the Reviewer
Let me know if a unit test is required or current setup is enough to ensure no regression since this seems pretty convoluted to write a straightforward test.
PR Checklist